Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feat] 7주차 과제 완료 #14

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

[Feat] 7주차 과제 완료 #14

wants to merge 7 commits into from

Conversation

imtaejugkim
Copy link
Contributor

Related issue 🛠

Work Description ✏️

  • MVI 패턴 적용

Screenshot 📸

전 주차 화면과 동일합니다!

Uncompleted Tasks 😅

  • Task1

To Reviewers 📢

과제 제출 기간을 잊어버렸네요... 죄송합니다...!

뷰모델과 State, SideEffect, Event를 컴포넌트화 하여 재사용하고, 행동을 정의할 수 있다는 것에 screen과 viewModel의 많은 코드 감소를 느낄 수 있었습니다! 본 과제 작동 방식이 맞는지 잘 몰라서 피드백 주시면 감사드립니다!

@imtaejugkim imtaejugkim added the enhancement New feature or request label Dec 18, 2024
@imtaejugkim imtaejugkim self-assigned this Dec 18, 2024
Copy link

@hyoeunjoo hyoeunjoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MVI가 참 어렵죠ㅠㅠ 이번주도 구현하시느냐 이번 기수 과제하느냐 고생 너무 많으셨습니다!!

Comment on lines +38 to +42
fun logIn() {
setState { copy(isLoading = true) }
viewModelScope.launch {
val state = uiState.value
val result = userRepository.postUserLogin(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기에 상태 업데이트와 비지니스 로직이 섞여있는 것 같아용 약간 분리해줘도 좋을 것 같은데요??

Comment on lines +9 to +11
val username: String = "",
val password: String = "",
val isLoading: Boolean = false,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요기서는 왜 모든 값에 기본값을 제공해주신걸꺼용?

Copy link

@sayyyho sayyyho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

너무 잘 하셔서 새롭게 배운 부분에 커멘트 남겼습니다! 고생하셨습니다!! 앱잼 파이팅!!

Comment on lines +46 to +53
when (effect) {
is SignUpSideEffect.ShowToast -> {
context.showToast(effect.message)
}
}

is RegisterState.Failure -> {
when (registerState.code) {
"00" -> context.showToast(R.string.sign_up_error)
"01" -> context.showToast(R.string.sign_up_eight)
else -> {}
is SignUpSideEffect.NavigateToLogin -> {
navController.navigate("login") {
popUpTo(0) { inclusive = true }
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확실히 이전에 비해 훨씬 더 깔끔해졌네요!

Comment on lines +13 to +38
abstract class BaseViewModel<State : UiState, SideEffect : UiSideEffect, Event : UiEvent> :
ViewModel() {

private val initialState: State by lazy { createInitialState() }
abstract fun createInitialState(): State

private val _uiState = MutableStateFlow(initialState)
val uiState: StateFlow<State> = _uiState.asStateFlow()

private val _sideEffect = MutableSharedFlow<SideEffect>()
val sideEffect: SharedFlow<SideEffect> = _sideEffect.asSharedFlow()

fun setState(reduce: State.() -> State) {
_uiState.value = _uiState.value.reduce()
}

fun setSideEffect(builder: () -> SideEffect) {
viewModelScope.launch { _sideEffect.emit(builder()) }
}

fun setEvent(event: Event) {
viewModelScope.launch { handleEvent(event) }
}

protected abstract suspend fun handleEvent(event: Event)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BaseViewModel을 두어서 하는 방식 좋네요!!

Copy link
Contributor

@jihyunniiii jihyunniiii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다 ~

import org.sopt.and.presentation.signup.components.IdHobbyTextField
import org.sopt.and.presentation.signup.components.SocialServiceLogIn
import org.sopt.and.util.showSnackBar

@Composable
fun LogInScreen(
navController: NavController,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

책임 분리를 위해서 navController 자체를 인자로 받기 보다는 이를 이용한 함수를 인자로 받는 게 좋을 것 같아요!
관련한 내용이 합동 세미나 피드백에 있으니 읽어보시면 좋을 것 같습니다 ~


override fun createInitialState(): LogInState = LogInState()

override suspend fun handleEvent(event: LogInEvent) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

event와 sideEffect의 차이는 뭘까요?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] 7주차 과제
4 participants